Resolve false negative for `used-before-assignment` with TYPE_CHECKING guard #9990

zenlyj · 2024-09-30T19:55:32Z

Type of Changes

	Type
✓	🐛 Bug fix
	✨ New feature
	🔨 Refactoring
	📜 Docs

Description

In the function responsible for filtering nodes defined under a TYPE_CHECKING block from consumption, usage scopes should only be updated when used-before-assignment is reported.

However, there is currently a misalignment between the _report_unfound_name_definition() and the filter function regarding when the error is raised. To resolve this, _report_unfound_name_definition() has been updated to return the check result, which is now passed to the filter function.

Additionally, the check has been extended to handle cases where a class is defined within an if block and used later.

Closes #8893

zenlyj · 2024-09-30T20:10:50Z

setting this to draft first, as i would like to check this more thoroughly. but feel free to discuss / give feedback!

codecov · 2024-09-30T20:19:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.80%. Comparing base (537e9da) to head (16d14e8).
Report is 64 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9990   +/-   ##
=======================================
  Coverage   95.80%   95.80%           
=======================================
  Files         174      174           
  Lines       18937    18939    +2     
=======================================
+ Hits        18143    18145    +2     
  Misses        794      794

Files with missing lines	Coverage Δ
pylint/checkers/variables.py	`97.20% <100.00%> (+<0.01%)`	⬆️

zenlyj · 2024-10-13T17:50:08Z

@jacobtylerwalls can i get a review for this PR?

regarding the primer result, the sentry one seems legitimate. as an import is made under TYPE_CHECKING, then used for an isinstance check. the django ones should be reasonable reports as well? where a class is defined within an if block, but not within the associated else block, hence raising possibly-used-before-assignment

jacobtylerwalls

Thank you! Just a small request to handle one more case.

pylint/checkers/variables.py

tests/functional/u/used/used_before_assignment_typing.py

jacobtylerwalls · 2024-10-14T13:47:11Z

Any idea about the new Django message for a nonlocal?

zenlyj · 2024-10-14T14:41:27Z

seems that the message was raised due to a guarded function definition further down at line 1584. but from my understanding, this is a false positive? it should not flag as condition is provided as a function parameter in the enclosing function _deferredSkip()

jacobtylerwalls · 2024-10-14T15:14:51Z

I wonder if we need to explicitly handle nonlocals somewhere. If you don't have bandwidth then I guess we can back out your last change and punt this to a new issue so that we don't cause more false positives.

zenlyj · 2024-10-14T15:35:47Z

perhaps we should drop the latest commit and track this case with a separate issue? so that we can unblock #9964.

you may assign the issue to me and i'll find time to work on it

jacobtylerwalls · 2024-10-14T16:29:07Z

Very good--thank you!

nickdrozd · 2024-10-16T13:32:25Z

There is also the problem of unused-import false negatives:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from math import pi  # unused-import false negative

Is it possible to fix that along the way?

jacobtylerwalls · 2024-10-16T14:15:37Z

you may assign the issue to me and i'll find time to work on it

Again, thanks for this offer. I opened #10028

for more information, see https://pre-commit.ci

zenlyj · 2024-10-17T13:02:13Z

you may assign the issue to me and i'll find time to work on it

Again, thanks for this offer. I opened #10028

hey no worries, i dropped the latest commit so this should be good to go.

There is also the problem of unused-import false negatives:
from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from math import pi  # unused-import false negative
Is it possible to fix that along the way?

sure thing, i added it to #10028

github-actions · 2024-10-17T13:18:22Z

🤖 Effect of this PR on checked open source code: 🤖

Effect on django:
The following messages are now emitted:

possibly-used-before-assignment:
Possibly using variable 'ServerBindingCursor' before assignment
https://github.com/django/django/blob/97c05a64ca87253e9789ebaab4b6d20a1b2370cf/django/db/backends/postgresql/base.py#L280
possibly-used-before-assignment:
Possibly using variable 'ServerSideCursor' before assignment
https://github.com/django/django/blob/97c05a64ca87253e9789ebaab4b6d20a1b2370cf/django/db/backends/postgresql/base.py#L416

Effect on sentry:
The following messages are now emitted:

used-before-assignment:
Using variable 'GroupEvent' before assignment
https://github.com/getsentry/sentry/blob/333e1ae42144f2d52a3f3e53b35cd792230f6073/src/sentry/eventstream/base.py#L146

This comment was generated for commit 16d14e8

Pierre-Sassoulas

The sentry primer looks legit.

zenlyj marked this pull request as draft September 30, 2024 19:56

jacobtylerwalls added this to the 4.0.0 milestone Sep 30, 2024

jacobtylerwalls added the False Negative 🦋 No message is emitted but something is wrong with the code label Sep 30, 2024

zenlyj force-pushed the fix/8893-fn-type-checking-guard branch from 638a16a to 4c5272e Compare September 30, 2024 20:09